Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(continuous-deploy-fingerprint): Add optional environment input for EAS updates #316

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tharakadesilva
Copy link
Contributor

@tharakadesilva
Copy link
Contributor Author

Hi @wschurman,

I'd appreciate your review on this PR. A couple of quick notes:

  1. The failing CI job appears unrelated to my changes.
  2. I'm still learning the approval request process - could you point me to any documentation or guidelines I should follow?

Thanks for your help!

@wschurman wschurman self-requested a review December 6, 2024 00:49
@wschurman
Copy link
Member

The failing CI job appears unrelated to my changes.

Yep, this happens for external contributions. No worries.

I'm still learning the approval request process - could you point me to any documentation or guidelines I should follow?

I added myself as a reviewer. I should have time to review in the next few days. I think we need to ensure that both this and the call here both use the same env variables since they could be in the app.json. And then we also probably need a way to guarantee that they are the same ones as the eas build profile uses?

I'll put some more thought into this in the next couple days.

@tharakadesilva
Copy link
Contributor Author

tharakadesilva commented Dec 6, 2024

I think we need to ensure that both this and the call here both use the same env variables since they could be in the app.json.

Ah, this is something that I missed, thanks for bringing this up!

I actually haven't referenced any variables in app.json in my setup (except SENTRY_TOKEN, but that is used at build time), which is probably why it worked in my use case that I described here.

This brings up an interesting question though: Should the fingerprint change when a JS bundle env var changes? For variables specified in app.json (like SENTRY_TOKEN), aren't these only used during build time rather than update publish time?

And then we also probably need a way to guarantee that they are the same ones as the eas build profile uses?

Based on the documentation, this seems to be already handled. The environment variables are made available based on the active profile, so no additional work should be needed on our end.

The main issue was with updates. With updates:

When the --environment flag is not provided, eas update will use the .env files present in your project directory for the update job and won't use the environment variables set on the EAS servers.

This explains why in my Supabase implementation, while the environment variables were available during build time, they weren't accessible via process.env during update publishing since they weren't present during that phase.

I'll put some more thought into this in the next couple days.

I really appreciate your feedback and time on this! And please excuse any misunderstandings on my part – I'm still quite new to this ecosystem.

Copy link
Member

This brings up an interesting question though: Should the fingerprint change when a JS bundle env var changes? For variables specified in app.json (like SENTRY_TOKEN), aren't these only used during build time rather than update publish time?

The fingerprint runtime version is used to declare precise compatibility between builds and updates, so if something changes the build runtime version the only way to guarantee compatibility is if the update runtime version is also changed. Anything that changes the build changes the fingerprint by default, though all users have the ability to exclude things from fingerprint if they wish via the provided exclusion mechanisms to set up custom rules for excluding certain changes from changing the fingerprint.

And then we also probably need a way to guarantee that they are the same ones as the eas build profile uses?
Based on the documentation, this seems to be already handled. The environment variables are made available based on the active profile, so no additional work should be needed on our end.

Sorry, I should have been more precise. I more meant guarantee/require that the "environment" string provided here matches the "environment" string in the eas.json profile. This way, if the build profile specifies an environment we can throw a warning if the "environment" flag you add isn't provided.

Copy link
Member

Asking internally how we recommend fixing this for EAS environment variables, will get back to you in a few days.

@wschurman
Copy link
Member

@tharakadesilva - I added some code that should theoretically load environment variables for execution of the npx expo config call and the npx expo-updates fingerprint:generate call. I haven't had time to test it yet though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants